Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update geometric.hpp #1127

Merged
merged 3 commits into from
Apr 30, 2024
Merged

Update geometric.hpp #1127

merged 3 commits into from
Apr 30, 2024

Conversation

kenarab
Copy link
Contributor

@kenarab kenarab commented Apr 29, 2024

The proposal is derived from this issue

#1126

This line should be corrected
At /math/distributions/geometric.hpp at line 404

  return std::log(p); // success_fraction

Now the code is

  return log(p); // success_fraction

When building that library. Throws this error

In file included from /opt/homebrew/include/boost/math/distributions.hpp:26:
/opt/homebrew/include/boost/math/distributions/geometric.hpp:404:16: error: unexpected namespace name 'log': expected expression
        return log(p); // success_fraction
               ^
1 error generated.
make[2]: *** [CMakeFiles/PonisIqCoreNLV.dir/Unity/unity_0_cxx.cxx.o] Error 1
make[1]: *** [CMakeFiles/PonisIqCoreNLV.dir/all] Error 2
make: *** [all] Error 2

@kenarab
Update geometric.hpp
52af702 
The proposal is derived from this issue
boostorg#1126
mborland
mborland requested changes Apr 29, 2024
View reviewed changes
Copy link
Member

@mborland mborland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

log should be found by ADL for any RealType. The correct fix would be adding using std::log; under this line: https://github.com/boostorg/math/blob/develop/include/boost/math/distributions/geometric.hpp#L386. I think it also needs a using std::exp;.

@kenarab
Update geometric.hpp
419f44a 
Implemented suggested change in
boostorg#1127 (review)
@kenarab kenarab requested a review from mborland April 29, 2024 20:12
@codecov Codecov
Copy link

codecov bot commented Apr 30, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.71%. Comparing base (28a5ffb) to head (1303786).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1127      +/-   ##
===========================================
+ Coverage    93.69%   93.71%   +0.01%     
===========================================
  Files          770      770              
  Lines        61057    61057              
===========================================
+ Hits         57209    57221      +12     
+ Misses        3848     3836      -12
Files Coverage Δ
include/boost/math/distributions/geometric.hpp 89.28% <ø> (ø)

... and 1 file with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28a5ffb...1303786. Read the comment docs.

@mborland
Copy link
Member

macOS clang failures are being addressed elsewhere

@mborland mborland merged commit 1399ad8 into boostorg:develop Apr 30, 2024
63 of 69 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mborland mborland mborland approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kenarab @mborland